Fix Kodi specific services registry and add descriptions#7551
Conversation
…ions - Fixes issue home-assistant#7528 - Add descriptions for Kodi specific services in services.yaml. - Error handling in Kodi API errors. - Make compatible the existent specific service `media_player.kodi_set_shuffle` with the general `media_player.shuffle_set` service (both use the same method but with different named parameter, I think the Kodi specific service should be eliminated, since it is not)
| out = self._find(album_name, [a['label'] for a in albums['albums']]) | ||
| return albums['albums'][out[0][0]]['albumid'] | ||
| try: | ||
| out = self._find(album_name, [a['label'] for a in albums['albums']]) |
There was a problem hiding this comment.
line too long (80 > 79 characters)
azogue
changed the title
emlove
left a comment
emlove
left a comment
There was a problem hiding this comment.
Looks good. Added a few notes.
|
|
||
| SERVICE_ADD_MEDIA = 'kodi_add_to_playlist' | ||
| SERVICE_SET_SHUFFLE = 'kodi_set_shuffle' | ||
| SERVICE_SET_SHUFFLE = 'kodi_set_shuffle' # this is the same as mp.shuffle_set |
There was a problem hiding this comment.
Let's just eliminate this. It only existed for a short while before core shuffle support was added.
| def async_setup_platform(hass, config, async_add_devices, discovery_info=None): | ||
| """Set up the Kodi platform.""" | ||
| if DATA_KODI not in hass.data: | ||
| hass.data[DATA_KODI] = [] |
There was a problem hiding this comment.
Let's instead make this a dict with the entity_id as the key, so we don't have to search it later.
| descriptions = yield from hass.loop.run_in_executor( | ||
| None, load_yaml_config_file, os.path.join( | ||
| os.path.dirname(__file__), 'services.yaml')) | ||
|
|
There was a problem hiding this comment.
I think we want to add a guard clause here to check if the services are already registered. Something like
if hass.services.has_service(DOMAIN, SERVICE_ADD_MEDIA):
return| for service in SERVICE_TO_METHOD: | ||
| schema = SERVICE_TO_METHOD[service].get( | ||
| 'schema', MEDIA_PLAYER_SCHEMA) | ||
| schema = SERVICE_TO_METHOD[service].get('schema') |
There was a problem hiding this comment.
You could even make this schema = SERVICE_TO_METHOD[service]['schema']. We want to ensure the schema is defined for future changes.
| vol.Optional(ATTR_MEDIA_ID): cv.string, | ||
| vol.Optional(ATTR_MEDIA_NAME): cv.string, | ||
| vol.Optional(ATTR_MEDIA_ARTIST_NAME): cv.string, | ||
| vol.Inclusive(ATTR_MEDIA_NAME, 'media_search'): cv.string, |
There was a problem hiding this comment.
Can we keep these optional, and default it to ALL in the service if looking for artists? Each media type should be able to make the best decision what to do if one isn't defined.
There was a problem hiding this comment.
Somehow this was missed before, but this is a coroutine. If we make this yield from async_add_devices(..., the entity_id should be available afterwards. Use entity.entity_id as the dict key.
There was a problem hiding this comment.
I think it is not possible, since v0.40, the passed in async_add_devices method is a callback instead of a coroutine function. (https://home-assistant.io/blog/2017/03/11/repurpose-any-android-phone-as-ip-camera/)
I also did not like having to use slugify in that place, maybe it would be better to leave hass.data[DATA_KODI] as a list?
There was a problem hiding this comment.
Ah, I missed that. In that case then yeah, going back to the list seems to make the most sense.
- Removed `kodi_set_shuffle` service. - Optional `media_name` and `artist_name` parameters. `media_name` defaults to 'ALL'. - Guard clause to check if the services are already registered.
ghost
added
7 participants
Description:
media_player.kodi_set_shuffleandmedia_player.kodi_add_to_playlist.media_player.kodi_set_shufflewith the generalmedia_player.shuffle_setservice (both use the same method but with different named parameter, I think the Kodi specific service should be eliminated, since it is not)SUPPORT_SHUFFLE_SETmedia player flag.Related issue (if applicable): fixes #7528
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2614
Checklist:
If user exposed functionality or configuration variables are added/changed: